Skip to content

Comments

Manchester | 26-ITP-Jan | Ahmed Elmahmoudi | Sprint 2 | Form-Controls#1177

Open
Alaterry8 wants to merge 15 commits intoCodeYourFuture:mainfrom
Alaterry8:Form-Controls
Open

Manchester | 26-ITP-Jan | Ahmed Elmahmoudi | Sprint 2 | Form-Controls#1177
Alaterry8 wants to merge 15 commits intoCodeYourFuture:mainfrom
Alaterry8:Form-Controls

Conversation

@Alaterry8
Copy link

Self checklist

  • I have titled my PR with Region | Cohort | FirstName LastName | Sprint | Assignment Title
  • My changes meet the requirements of the task
  • I have tested my changes
  • My changes follow the style guide

Within a form, I added a name field with minimum 2 characters length requirement. I added an email field. I added a set of 3 colours to select from. I added 6 radio options for Size options.

@netlify
Copy link

netlify bot commented Feb 14, 2026

Deploy Preview for cyf-onboarding-module ready!

Name Link
🔨 Latest commit 3063de4
🔍 Latest deploy log https://app.netlify.com/projects/cyf-onboarding-module/deploys/6998c4fdf1b2ae00080bdf9c
😎 Deploy Preview https://deploy-preview-1177--cyf-onboarding-module.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
2 paths audited
Performance: 100 (no change from production)
Accessibility: 100 (no change from production)
Best Practices: 100 (no change from production)
SEO: 86 (no change from production)
PWA: -
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify project configuration.

@Alaterry8 Alaterry8 added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Feb 14, 2026

<br> <br>
<label for="colour"> T-shirt colour:</label>
<select id="colour" name="T-shirt colour">

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note, we must require the user to actively select colour. Therefore initial option should be empty and we must demand the choise.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed this by adding an empty option value, and made it required to choose a colour. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could be weird to see an empty choice in a dropdown list:
image

Could you make it more explicit to the user that they should choose a colour?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback, I added an indication text.

try writing out the requirements first as comments
this will also help you fill in your PR message later-->
<label for="name"> Name: </label>
<input type="text" id="name" name="Name" minlength="2" required>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The convention for naming elements (i.e' name) is lowercase

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so the only thing that can be capitalised is what goes between the label tags?

Copy link
Contributor

@cjyuan cjyuan Feb 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They serve different purposes. The text in the label is what users see on their browser. The value of name is intended to be consumed by a server-side script.

Can you use an AI tool to find out why it is better to express value of names in lowercase?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I understand now. Thanks.


<br> <br>
<label for="colour"> T-shirt colour:</label>
<select id="colour" name="T-shirt colour">

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the convention for name is to avoid usage spaces. Use underscore or dash instead

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noted! fixed it. Thanks for all the feedback ;D

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why use t-shirt_colour for one input and just size (without t-shirt) for the other input?

Either format is acceptable, but it is better to keep the format consistent.

The same also applies to the labels.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cjyuan That makes sense, I made the appropriate changes. Thanks for the pointers!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was referring to name="t-shirt_colour" on line 27 and name="size" on the radio buttons.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cjyuan Yea I understand the issue now, I changed it so it's more consistent

@SlideGauge SlideGauge added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Feb 15, 2026
@Alaterry8 Alaterry8 added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. 📅 Sprint 2 Assigned during Sprint 2 of this module Module-Onboarding The name of the module. and removed Reviewed Volunteer to add when completing a review with trainee action still to take. labels Feb 16, 2026
@Alaterry8
Copy link
Author

Alaterry8 commented Feb 18, 2026

Hi @cjyuan @SlideGauge can you kindly review my changes, Thanks!

@cjyuan
Copy link
Contributor

cjyuan commented Feb 18, 2026

@Alaterry8 You have done a good job in addressing all of the reviewer's comments. I just have some more suggestions.

@cjyuan cjyuan added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Feb 18, 2026
@Alaterry8 Alaterry8 added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. and removed Reviewed Volunteer to add when completing a review with trainee action still to take. labels Feb 19, 2026
@Alaterry8
Copy link
Author

@cjyuan @SlideGauge Thanks for all the feedback, I made the suggested changes.

@cjyuan
Copy link
Contributor

cjyuan commented Feb 19, 2026

Can you address this #1177 (comment)? Other changes look good.

@cjyuan cjyuan added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Feb 19, 2026
@Alaterry8
Copy link
Author

@cjyuan I made the changes, thanks for the feedback!

@Alaterry8 Alaterry8 added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. and removed Reviewed Volunteer to add when completing a review with trainee action still to take. labels Feb 20, 2026
@cjyuan
Copy link
Contributor

cjyuan commented Feb 20, 2026

Can you push your changes to GitHub?

@Alaterry8
Copy link
Author

@cjyuan I did

@cjyuan
Copy link
Contributor

cjyuan commented Feb 20, 2026

No new commits made on this branch since yesterday. Did you made your changes on a different branch?

If you have made commit on your computer, you need to pusn/sync them to GitHub.

@Alaterry8
Copy link
Author

@cjyuan it should be synced now

@Alaterry8
Copy link
Author

@cjyuan How do I know that it is synced, where do the changes show on github?

@cjyuan
Copy link
Contributor

cjyuan commented Feb 20, 2026

Changes look good.

Note: To check if your latest commits were sync, you can check the commits history made on the branch.

@cjyuan cjyuan added Complete Volunteer to add when work is complete and all review comments have been addressed. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Feb 20, 2026
@Alaterry8
Copy link
Author

Thanks for all the valuable help and feedback! @cjyuan @SlideGauge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Complete Volunteer to add when work is complete and all review comments have been addressed. Module-Onboarding The name of the module. 📅 Sprint 2 Assigned during Sprint 2 of this module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants